-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Close #80, Add build number and baseline #86
Close #80, Add build number and baseline #86
Conversation
Are multi-line events allowed? |
Who can we ask? Or where can we check? They print out fine on the console and output file. |
I'd be more comfortable with following the standard one line format unless there is a critical reason to change. I'm not aware of any other event anywhere that has new lines. Could impact ground stations, event processing/handling tools, etc. I wouldn't think the effort to verify multi-line doesn't break anything across all the ground test and ops tools/teams is worth it in this case. |
Fair enough. I did it because it is more readable. Maybe bring it up at the CCB and see what others think? |
I would concur with sticking to a single line. The FSW doesn't care - EVS passes it through no problem - but ground system might not display multiple lines correctly. |
I think it's worthwhile to check if the different ground systems can handle it. I can keep it as one line for now though. |
9a45d32
to
cb48edc
Compare
I agree with keeping it on one line. I'm not sure I like it on the console, since I am used to looking at messages from an app in a single line. Also don't know if it will trip up ground system parsing. |
5723664
to
dc57b7a
Compare
If the ground station trips up then that's definitely a problem. \begin-rant I know we're used to seeing things a certain way. I just want to point out that we want to make sure that messages stand out and are obvious. This is especially important as we aim to grow the cFS user base. The one-line event now gets buried. And thus the message is lost to the user.
Text should be formatted for humans. Spaces and line breaks help us process information more effectively. \end-rant |
Add build number and baseline macros Add version string macros Use new version string in noop and version report
dc57b7a
to
03f4efa
Compare
FWIW, the version info should really be a separate event from the startup info. At startup, it can send both, but the NOOP command only sends the version info event. That would put them on separate lines (startup + version info) and also keep each event to one line. |
I just followed the existing pattern but it sounds like it's worth checking that all the sample and lab apps are using the noop to communicate the version. |
Also noticed that some apps are using |
CFE ES does it (more) properly - it defines a separate I do concur that the version info being "buried" inside other event strings is not ideal - but I would prefer to see separate event IDs, rather than having one event span multiple lines. |
Describe the contribution
Close #80
Testing performed
Built bundle and confirmed SAMPLE_APP reports development version.
Expected behavior changes
Version report now uses the version string. See excerpt from cfs log:
System(s) tested on
Ubuntu Docker on Mac OS X
Additional context
Add any other context about the contribution here.
Third party code
None
Contributor Info - All information REQUIRED for consideration of pull request
Gerardo E. Cruz-Ortiz, NASA-GSFC